EVG-19946: Support Docker on provider settings page #2017

minnakt · 2023-09-01T15:09:52Z

EVG-19946

Description

This PR adds Docker to the provider settings page. The Docker configuration is kind of weird (container pool ID is not part of provider settings list) so I made some changes in the form schema and conversion functions.

Screenshots

Testing

Cypress tests, Jest tests

Evergreen PR

EVG-19946: Expose container pool settings via SpruceConfig evergreen#6984

sophstad · 2023-09-06T14:46:45Z

cypress/integration/distroSettings/provider_section.ts

-    cy.getInputByLabel("User Data").type("my user data");
-    cy.getInputByLabel("Merge with existing user data").check({
-      force: true,
+  describe.only("docker", () => {


Suggested change

describe.only("docker", () => {

describe("docker", () => {

sophstad · 2023-09-06T14:47:34Z

src/gql/queries/index.ts

@@ -71,6 +70,7 @@ import PROJECT_HEALTH_VIEW from "./project-health-view.graphql";
 import GET_PROJECT_PATCHES from "./project-patches.graphql";
 import GET_SPAWN_EXPIRATION_INFO from "./spawn-expiration.graphql";
 import GET_SPAWN_TASK from "./spawn-task.graphql";
+import GET_SPRUCE_CONFIG from "./spruce-config.graphql";


We've been removing GET_ from the import/export name too

Suggested change

import GET_SPRUCE_CONFIG from "./spruce-config.graphql";

import SPRUCE_CONFIG from "./spruce-config.graphql";

sophstad · 2023-09-06T15:22:40Z

src/pages/distroSettings/tabs/ProviderTab/transformers.ts

+          providerName: Provider.Docker,
+        },
+        providerSettings: {
+          ...dockerProviderSettings(providerSettingsList[0]).form,


I switched the localhost distro to use Docker and then tried to switch it back to static and saw this error:

It seems like there's validation here preventing container pool distros from using a different provider.

Maybe someone on app would have insight into the best approach here. Do we want to ban Docker distros from having their types changed? Or do we maybe want to update the save function to remove this distro from the list of Docker distros? 🤔

oh that's probably my fault. Have to clear the containerPool field in the transformers

sophstad · 2023-09-06T15:39:58Z

src/pages/distroSettings/tabs/ProviderTab/transformers.ts

+        providerSettingsList: [
+          ...dockerProviderSettings(data.providerSettings).gql,
+        ],
+        // @ts-ignore-error - containerPoolId will exist in DockerFormState.


We can narrow down the type here using the in operator to avoid this error (docs). Something like this:

case Provider.Docker: { if ("containerPoolId" in data.providerSettings) { return { ...distro, provider: Provider.Docker, providerSettingsList: [ ...dockerProviderSettings(data.providerSettings).gql, ], containerPool: data.providerSettings.containerPoolId, }; } break; }

SupaJoon · 2023-09-08T17:12:19Z

evergreen retry

SupaJoon · 2023-09-08T16:30:21Z

src/pages/distroSettings/tabs/ProviderTab/ProviderTab.tsx

+  const { pools = [] } = containerPools || {};
+
+  const selectedPoolId = formData?.providerSettings?.containerPoolId;
+  const selectedPool = pools.find((p) => p.id === selectedPoolId) ?? null;
+  const poolMappingInfo = selectedPool
+    ? JSON.stringify(omitTypename(selectedPool), null, 4)
+    : "";
+
+  const formSchema = useMemo(
+    () => getFormSchema({ pools, poolMappingInfo }),
+    [pools, poolMappingInfo]
+  );


The useMemo will run on every render when pools is undefined. You can avoid doing that by casting pools to an array closer to where it's used. Another place to cast pools is in getFormSchema which is good if the function were reused so we only have to type cast in one place.

Suggested change

const { pools = [] } = containerPools || {};

const selectedPoolId = formData?.providerSettings?.containerPoolId;

const selectedPool = pools.find((p) => p.id === selectedPoolId) ?? null;

const poolMappingInfo = selectedPool

? JSON.stringify(omitTypename(selectedPool), null, 4)

: "";

const formSchema = useMemo(

() => getFormSchema({ pools, poolMappingInfo }),

[pools, poolMappingInfo]

);

const { pools } = containerPools || {};

const selectedPoolId = formData?.providerSettings?.containerPoolId;

const selectedPool = pools?.find((p) => p.id === selectedPoolId) ?? null;

const poolMappingInfo = selectedPool

? JSON.stringify(omitTypename(selectedPool), null, 4)

: "";

const formSchema = useMemo(

() => getFormSchema({ pools: pools || [], poolMappingInfo }),

[pools, poolMappingInfo]

);

cypress · 2023-09-13T20:18:44Z

Passing run #12763 ↗︎

0	584	7	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

Update SpruceConfig mock to contain container pools
Project: Spruce	Commit: `e3ee2db36d`
Status: Passed	Duration: 17:54 💡
Started: Sep 18, 2023 9:02 PM	Ended: Sep 18, 2023 9:20 PM

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

minnakt · 2023-09-14T01:17:17Z

hello hello, so after experimenting a bit with the EC2 providers, I ended up concluding that maybe we should just have unique fields for each provider (rather than trying to share the providerSettings field across all four providers). This solution will require us to redeclare some fields, but I think it might be simpler overall.

I first tried a more "elegant" solution where the providerSettings field would be an array in the schema, and we would populate the array with the appropriate amount of items (1 item for Static, 1 item for Docker, 4 items for EC2). This didn't work out so well due to a specific scenario:

Start out with EC2 provider with 4 items in the array (each item corresponds to an AWS region)
Switch to the Static provider
You'd still have 4 items in the array, which is wrong for Static providers

And this is why I ended up thinking that it would be simpler for each of the providers to have their own fields. Lmk what you think! 😲

sophstad · 2023-09-15T14:11:07Z

src/pages/distroSettings/tabs/ProviderTab/types.ts

+  Pull = "pull",
+}
+
+// TODO: Append type with additional provider options, e.g. type ProviderFormState = StaticProviderFormState | DockerProviderFormState


[nit] I think this no longer applies in the way it's written here, could update or just remove.

sophstad · 2023-09-15T14:18:32Z

src/pages/distroSettings/tabs/ProviderTab/ProviderTab.tsx

 import { BaseTab } from "../BaseTab";
 import { getFormSchema } from "./getFormSchema";
-import { TabProps } from "./types";
+import { TabProps, ProviderFormState } from "./types";

 export const ProviderTab: React.FC<TabProps> = ({ distroData }) => {
  const initialFormState = distroData;


[nit] No need to rename this var (even though I think we have been doing it unnecessarily in every tab 😂)

sophstad · 2023-09-15T14:22:53Z

src/pages/distroSettings/tabs/ProviderTab/transformers.test.ts

-    },
-  ],
-};
+  describe("docker provider", () => {


Would you mind adding a test that uses the poolMappingInfo field?

sophstad · 2023-09-15T14:23:41Z

src/pages/distroSettings/tabs/ProviderTab/types.ts

+    buildType: BuildType;
+    registryUsername: string;
+    registryPassword: string;
+    containerPoolId: string;


It seems like poolMappingInfo could also be a field here no?

SupaJoon · 2023-09-15T17:23:51Z

src/pages/distroSettings/tabs/ProviderTab/transformerUtils.ts

@@ -30,27 +32,88 @@ const getSecurityGroups = ((providerSettings) => ({
  },
 })) satisfies FieldGetter;

+const getImageUrl = ((providerSettings) => ({


Could we somehow indicate that providerSettings represents 2 different types of objects with TypeScript types? It will also be helpful if we can type the providerSettings from the API based on usage in Spruce so we know what to expect from the object.

sophstad · 2023-09-18T14:26:17Z

src/pages/distroSettings/tabs/ProviderTab/transformerUtils.ts

+type ProviderSettings = ProviderFormState["staticProviderSettings"] &
+  ProviderFormState["dockerProviderSettings"];
+
+export const gqlProviderSettings = (


This structure makes a lot more sense 😅 gorgeous!

* Because if you switch from static -> docker you need to update a bunch of fields, versus switch from docker -> static

SupaJoon

Well done!!! 😎

EVG-19946: Add Docker to provider settings page

46ae0f3

minnakt marked this pull request as ready for review September 1, 2023 16:09

minnakt requested a review from a team September 1, 2023 16:09

sophstad suggested changes Sep 6, 2023

View reviewed changes

SupaJoon suggested changes Sep 8, 2023

View reviewed changes

minnakt added 3 commits September 12, 2023 13:56

Merge branch 'main' into EVG-19946

32cd128

Merge branch 'main' into EVG-19946

d7d501d

Change form schema and address feedback

07b316c

minnakt requested review from sophstad and SupaJoon September 14, 2023 01:17

sophstad reviewed Sep 15, 2023

View reviewed changes

minnakt added 2 commits September 15, 2023 10:27

Merge branch 'main' into EVG-19946

9383586

Address comments

405ce37

SupaJoon reviewed Sep 15, 2023

View reviewed changes

minnakt added 2 commits September 16, 2023 14:30

Surface types

0a7ea2f

Prevent undefined error

2f14006

minnakt requested review from sophstad and SupaJoon September 18, 2023 13:56

sophstad approved these changes Sep 18, 2023

View reviewed changes

minnakt added 2 commits September 18, 2023 12:35

Merge branch 'main' into EVG-19946

9c5beb2

Use a different distro for test

8e32f60

* Because if you switch from static -> docker you need to update a bunch of fields, versus switch from docker -> static

SupaJoon approved these changes Sep 18, 2023

View reviewed changes

minnakt added 2 commits September 18, 2023 16:45

Merge branch 'main' into EVG-19946

f2ab0cc

Update SpruceConfig mock to contain container pools

e3ee2db

minnakt changed the title ~~EVG-19946: Add Docker to provider settings page~~ EVG-19946: Support Docker on provider settings page Sep 18, 2023

minnakt changed the title ~~EVG-19946: Support Docker on provider settings page~~ EVG-19946: Add support for Docker on provider settings page Sep 18, 2023

minnakt changed the title ~~EVG-19946: Add support for Docker on provider settings page~~ EVG-19946: Support Docker on provider settings page Sep 18, 2023

minnakt merged commit 372a1e0 into evergreen-ci:main Sep 18, 2023
2 checks passed

minnakt deleted the EVG-19946 branch September 18, 2023 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EVG-19946: Support Docker on provider settings page #2017

EVG-19946: Support Docker on provider settings page #2017

minnakt commented Sep 1, 2023 •

edited

Loading

sophstad Sep 6, 2023

sophstad Sep 6, 2023

sophstad Sep 6, 2023

minnakt Sep 7, 2023

sophstad Sep 6, 2023

SupaJoon commented Sep 8, 2023

SupaJoon Sep 8, 2023

cypress bot commented Sep 13, 2023 •

edited

Loading

minnakt commented Sep 14, 2023

sophstad Sep 15, 2023

sophstad Sep 15, 2023

sophstad Sep 15, 2023

sophstad Sep 15, 2023

SupaJoon Sep 15, 2023

sophstad Sep 18, 2023

SupaJoon left a comment

	import GET_SPRUCE_CONFIG from "./spruce-config.graphql";
	import SPRUCE_CONFIG from "./spruce-config.graphql";

EVG-19946: Support Docker on provider settings page #2017

EVG-19946: Support Docker on provider settings page #2017

Conversation

minnakt commented Sep 1, 2023 • edited Loading

Description

Screenshots

Testing

Evergreen PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SupaJoon commented Sep 8, 2023

Choose a reason for hiding this comment

cypress bot commented Sep 13, 2023 • edited Loading

Passing run #12763 ↗︎

minnakt commented Sep 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SupaJoon left a comment

Choose a reason for hiding this comment

minnakt commented Sep 1, 2023 •

edited

Loading

cypress bot commented Sep 13, 2023 •

edited

Loading